Skip to content

[fix][evaluation] call evaluator with target reentering#470

Open
lsy357 wants to merge 11 commits intomainfrom
fix/expt
Open

[fix][evaluation] call evaluator with target reentering#470
lsy357 wants to merge 11 commits intomainfrom
fix/expt

Conversation

@lsy357
Copy link
Copy Markdown
Collaborator

@lsy357 lsy357 commented Mar 23, 2026

What type of PR is this?

fix

Check the PR title

  • This PR title match the format: [<type>][<scope>] <description>. For example: [fix][backend] flaky fix
  • The description of this PR title is user-oriented and clear enough for others to understand.
  • Add documentation if the current PR requires user awareness at the usage level.
  • This PR is written in English. PRs not in English will not be reviewed.

(Optional) Translate the PR title into Chinese

(Optional) More detailed description for this PR(en: English/zh: Chinese)

en:
zh(optional):

(Optional) Which issue(s) this PR fixes

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 23, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 86.15385% with 9 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
backend/infra/db/security.go 84.21% 6 Missing ⚠️
...aluation/domain/service/expt_run_item_turn_impl.go 75.00% 1 Missing and 1 partial ⚠️
...nd/api/router/coze/loop/apis/middleware/session.go 0.00% 1 Missing ⚠️

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #470   +/-   ##
=======================================
  Coverage   74.45%   74.46%           
=======================================
  Files         629      630    +1     
  Lines       66337    66396   +59     
=======================================
+ Hits        49389    49439   +50     
- Misses      13663    13670    +7     
- Partials     3285     3287    +2
Flag Coverage Δ
unittests 74.46% <86.15%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
backend/modules/evaluation/domain/entity/event.go 75.75% <100.00%> (+12.12%) ⬆️
...nfra/repo/experiment/ck/expt_turn_result_filter.go 66.84% <100.00%> (+0.54%) ⬆️
...nd/api/router/coze/loop/apis/middleware/session.go 3.44% <0.00%> (+0.11%) ⬆️
...aluation/domain/service/expt_run_item_turn_impl.go 87.50% <75.00%> (-0.34%) ⬇️
backend/infra/db/security.go 84.21% <84.21%> (ø)

... and 3 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88fdec6...df6fd00. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

CozeLoop
CozeLoop suggested changes Mar 23, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@CozeLoop CozeLoop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the evaluation fix looks reasonable (ctx-based target-called marker + updated call sites), and the added ctxcache tests help.

Requesting changes mainly around: (1) avoid mutating filter inputs when escaping SQL map keys; (2) add unit tests for the new SQL escaping helper (security-critical); (3) please confirm and document the auth behavior change for reset_password; plus a minor outdated comment in a test.

tpfz
tpfz previously approved these changes Mar 23, 2026
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tpfz tpfz tpfz left review comments

+1 more reviewer

@CozeLoop CozeLoop CozeLoop requested changes

Reviewers whose approvals may not affect merge requirements

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lsy357 @tpfz @CozeLoop